Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cases] Custom Fields as Cases Filters #171176

Merged

Conversation

jcger
Copy link
Contributor

@jcger jcger commented Nov 14, 2023

Meta issue: #167651

Summary

Screenshot.mov

What this PR does not include (will be done in future PRs)

  • If the users adds to many filters, the UI will overflow
  • If the url contains status or severity set and the filter is not active, we need to automatically activate that filter and set the filter option

useEffect situation

We tried to remove some useEffects, specifically in useFilterConfig and useSystemFilterConfig but as they have dependencies that are loaded from API's, there are some use cases, like if a new custom field is added or removed where effects (I think) are a must. We will come back to this issue once we have the feature in main to try to solve this issue

@jcger jcger added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature release_note:feature Makes this part of the condensed release notes v8.12.0 labels Nov 14, 2023
@jcger jcger added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:feature Makes this part of the condensed release notes labels Nov 16, 2023
@@ -129,7 +129,7 @@ export interface AppMockRenderer {
render: UiRender;
coreStart: StartServices;
queryClient: QueryClient;
AppWrapper: React.FC<{ children: React.ReactElement }>;
AppWrapper: React.FC<{ children: React.ReactNode }>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me. Unrelated to the type change, what about wrapper: appMockRender.AppWrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but it returns another ts error

Type 'FC<{ children: ReactNode; }>' is not assignable to type 'WrapperComponent<{ systemFilterConfig: FilterConfig[]; onFilterOptionChange: FilterChangeHandler; }> | undefined'.

Any idea what it means?

@@ -161,4 +161,23 @@ describe('multi select filter', () => {
await waitForEuiPopoverOpen();
expect(screen.getAllByTestId(TEST_ID).length).toBe(2);
});

it('should not show the amount of options if hideActiveOptionsNumber is active', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new feature as the "more" button doesn't seem to be showing this info

query: {
...DEFAULT_QUERY_PARAMS,
expect(fetchMock).toHaveBeenCalledWith(`${CASES_INTERNAL_URL}/_search`, {
method: 'POST',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

migration from _find to _search using post and body as string

@@ -257,4 +309,459 @@ describe('CasesTableFilters ', () => {
expect(onCreateCasePressed).toHaveBeenCalledWith();
});
});

describe('toggle type custom field filter', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the tests for this feature are here. This way we test the whole integration with the table

@jcger jcger marked this pull request as ready for review November 27, 2023 12:13
@jcger jcger requested a review from a team as a code owner November 27, 2023 12:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great code! I really liked the architecture and how clean it is. Some comments:

  1. If you add two custom fields, filter both of them by some value, delete them in another tab, and then switch to the tab where you filtered you will see this error:
Screenshot 2023-11-28 at 12 59 32 PM

Probably because two API calls are happening and the first one contains the second custom field

  1. I was playing around with creating multiple custom fields and deleting them and I managed to produce this bug. I don't know how. I cannot reproduce it 🙂
Screenshot 2023-11-28 at 1 08 34 PM
  1. I think we should remove the +more button from the modal. Wdyt?
  2. When I search for In progress it does not show. I guess we are searching in the keys, right?
Screenshot 2023-11-27 at 5 49 33 PM

x-pack/plugins/cases/common/ui/types.ts Show resolved Hide resolved
@@ -129,7 +129,7 @@ export interface AppMockRenderer {
render: UiRender;
coreStart: StartServices;
queryClient: QueryClient;
AppWrapper: React.FC<{ children: React.ReactElement }>;
AppWrapper: React.FC<{ children: React.ReactNode }>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me. Unrelated to the type change, what about wrapper: appMockRender.AppWrapper?

x-pack/plugins/cases/public/containers/api.ts Show resolved Hide resolved
@jcger
Copy link
Contributor Author

jcger commented Nov 29, 2023

Great code! I really liked the architecture and how clean it is. Some comments:

  1. If you add two custom fields, filter both of them by some value, delete them in another tab, and then switch to the tab where you filtered you will see this error:
    Probably because two API calls are happening and the first one contains the second custom field
  2. I was playing around with creating multiple custom fields and deleting them and I managed to produce this bug. I don't know how. I cannot reproduce it 🙂
  3. I think we should remove the +more button from the modal. Wdyt?
  4. When I search for In progress it does not show. I guess we are searching in the keys, right?
  1. Should be fixed now, after your idea, instead of updating one by one, each filter will return its part of the final filter so we can merge everything together and do just one call. This is the config and here and here you will find how it's being applied. I also changed the filter on change function to work with partial filter options and in the system config hook and custom field config hook is where we convert the multi select component output to our filter options
  2. Solved with this change also added test
  3. It is not rendered anymore here and the custom field config is not build
  4. We were searching in label but the label wasn't the right one, thanks to this change and some fine tuning to allow the key to be CaseStatuses type and not only string, it should be working now. I added a test for it

@kibana-ci
Copy link
Collaborator

kibana-ci commented Nov 29, 2023

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [1e68656]

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and all the issues are fixed now 🚀. Code LGTM. Thanks for addressing our feedback. I found the following small issues. We can fix them on another PR:

  1. The search payload contains empty custom fields. I think is better if we omit the empty object.
Screenshot 2023-11-29 at 5 31 54 PM
  1. Every time you delete 2+ custom fields two calls are being made with the one failing but no toaster is shown (probably due to the call being canceled immediately). I think you mentioned the same in our meeting.
Screenshot 2023-11-29 at 5 26 56 PM

@jcger jcger merged commit 219cbbd into elastic:cases/167651 Nov 30, 2023
26 of 29 checks passed
jcger added a commit that referenced this pull request Dec 4, 2023
…lds (#172276)

Meta issue #167651
Fixes: #167651

## Summary
Previous PRs merged into this feature branch:
- #169356
- #169371
- #170851
- #171102
- #171176

## Release notes
Case list filter bar can now be customised. Filters can be removed and
custom fields can be used as filters

## Pending issues
- Table in modal shouldn’t load in local storage saved filter options of
status/severity
- Status & Severity filters in url. Filters must be activated if the
user has them deactivated
- UI overflow when to much filters are active
- Race condition: When a user has a custom field active with an option
selected and this custom field gets removed in settings, it includes the
removed custom field when refreshing. This request will fail, triggering
a second one which won't include the removed custom field
- Found during QA. In the modal, when trying to select all options in
the solutions filter, when checking the last unchecked option, it resets
and there is no checked option anymore

## Flaky test runner link

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4128

---------

Co-authored-by: Antonio <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants